Skip to content

fix(policy): keep an explicitly saved default as an enforceable system rule - #9039

Open
maia-andre wants to merge 4 commits into
LibreSign:mainfrom
maia-andre:fix/system-policy-explicit-default-8916
Open

maia-andre wants to merge 4 commits into
LibreSign:mainfrom
maia-andre:fix/system-policy-explicit-default-8916

Conversation

@maia-andre

Copy link
Copy Markdown
Contributor

Resolves: #8916

📝 Summary

An administrator could not enforce the built-in default of a policy. Saving it with allowChildOverride = false was lost on both sides:

  • Write: PolicySource::saveSystemPolicy() deleted the system rule when the value equaled the default and the override was disabled.
  • Read: PolicySource::loadSystemPolicy() returned an overridable layer for an explicit default, whatever its flag. A group or user rule still changed the effective value.

What changes:

  • saveSystemPolicy() stores every explicit value with its override flag, including a value equal to the default. The special case that deleted the rule, including its JSON comparison, is gone.
  • loadSystemPolicy() applies the stored flag to an explicit default. A default stored without a flag predates explicit defaults, when the API could not enforce one, so it stays overridable (as agreed in the issue).
  • PolicyService::saveSystem(): null now means "no explicit system rule" and clears it (clearSystem()), instead of being turned into the default and saved.
    • Compound system writes go through the same path, so null and an explicit default differ there too. The composite validation already treated null as the default, which is what the effective value becomes.
    • Policy Workbench already removes a system rule by posting value: null, so the frontend needs no change. Saving the default with "do not allow overrides" now just works.
  • API documentation: both system write endpoints now describe null as removing the explicit rule. Before, they said it reset the value to its default.

Direct reads and bulk loading share loadSystemPolicy() (resolveMany() goes through resolveCore()), so they stay consistent. Rule counts already come from the stored key, so an enforced default is counted (existing testLoadAllRuleCountsCountsExplicitSystemRuleEvenWhenValueMatchesDefault).

I also checked the internal callers of saveSystem():

  • The background, reminder, footer and DocMDP services pass the current override flag, so their behavior does not change.
  • AEngineHandler (engine none) saves a value different from the default, which was already an enforced rule.
  • SignatureTextService::save() has no production caller.

Composition (2 commits): the fix is 70 lines of production code and 209 lines of tests. The regenerated OpenAPI/TypeScript commit is 16 lines, descriptions only.

🧪 How to test

  1. POST /ocs/v2.php/apps/libresign/api/v1/policies/system/show_confetti_after_signing with value = true, allowChildOverride = false (the default value, enforced).
  2. As admin, PUT …/policies/user/signer1/show_confetti_after_signing with value = false.
  3. As signer1, GET …/policies/effective: show_confetti_after_signing is true from global. On main it is false from user_policy.
  4. POST …/policies/system/show_confetti_after_signing with value = null, then the same GET as signer1: now false from user_policy.

This is the new scenario in features/policies/boolean_policy_layers.feature. It fails on main at step 3.

Validation evidence

  • Focused regression/acceptance test:
    • PolicySourceTest resolves the issue's matrix through DefaultPolicyResolver, against a conflicting group layer and a conflicting user layer, checking the effective value. The matrix covers no rule, an explicit default enforced or overridable, an explicit value enforced or overridable, and a legacy default without flag.
    • A save-then-read test checks the stored rule.
    • PolicyServiceTest: null clears and an explicit default saves, also in a compound write that mixes both.
    • Behat: the scenario above. The existing tests that expected the deletion now expect an enforced rule.
  • Relevant lint/type/static-analysis checks: composer cs:check is clean, and psalm adds no issue.
  • Broader suite required by the changed surface: the whole PHPUnit unit suite (4088 tests) and features/policies/boolean_policy_layers.feature (4 scenarios) pass.
  • CI reviewed

⚙️ API / Back-end changes

  • An explicit system default keeps its override flag. null removes the explicit system rule (single and compound writes).
  • Unit and/or integration tests added or updated
  • Authorization and negative paths checked when applicable: a lower user rule is ignored under an enforced default and applies again once the rule is removed
  • Capabilities updated when applicable: not applicable
  • Documentation updated when applicable: endpoint descriptions
  • API documentation regenerated with composer openapi when necessary

✅ Checklist

  • I have read and followed the contribution guide.
  • The diff does not contain unrelated refactors or generated changes.
  • Commits follow Conventional Commits where applicable.
  • All commits include the required DCO sign-off.

🤖 AI (if applicable)

  • The content of this PR was partially or fully generated using AI

…m rule

An administrator could not enforce the built-in default of a policy.
Saving the default with allowChildOverride = false deleted the system
rule, and reading an explicit default always returned an overridable
layer, so a group or user rule still changed the effective value.

- PolicySource::saveSystemPolicy() stores every explicit value with its
  override flag, including a value equal to the default.
- PolicySource::loadSystemPolicy() applies the stored flag to an
  explicit default. A default stored without a flag predates explicit
  defaults, when the API could not enforce one, so it stays
  overridable.
- PolicyService::saveSystem() treats null as "no explicit system rule"
  and clears it, for single and compound writes. Policy Workbench
  already removes a rule this way, so it needs no change.

The API documentation of both system write endpoints now states the
difference between null and an explicit default.

Resolves: LibreSign#8916
Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
Part of LibreSign#8916
Assisted-by: Claude Code:claude-opus-5-5

Signed-off-by: André Maia <andrefnkmm@gmail.com>
@maia-andre
maia-andre requested a review from a team as a code owner October 6, 2026 01:27
Saving a value equal to the default now keeps it as an explicit system
rule, so the admin initial state scenarios and the validation access
end-to-end test can no longer use the default value to clear the rule.
They send null, which is how the workbench removes a system rule.

Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
@YvesCesar
YvesCesar self-requested a review October 17, 2026 16:44

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @maia-andre ! :)

Can you check this review?

Comment thread lib/Service/Policy/Runtime/PolicySource.php Outdated
An empty stored value was always read as "no explicit rule", so saving
legal_information (default '') as enforced left a group or user layer in
control and orphaned its override flag. The policy API writes the flag
with every value, so only an empty value without it is treated as absent.

Signed-off-by: André Maia <andrefnkmm@gmail.com>
@maia-andre
maia-andre requested a review from YvesCesar October 17, 2026 21:52

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this resolves what I raised: the read side is fixed and the new matrix covers it. I ran the Policy unit tests on 481967c: 972 pass.

With the read fixed, one related spot now gets out of sync (I missed it in my first comment): the rule count still ignores empty values.

->andWhere($systemConfigQuery->expr()->neq('configvalue', $systemConfigQuery->createNamedParameter('')));

hydrateExplicitSystemRuleCounts() filters configvalue != '', so an enforced legal_information = '' is active but gets everyoneCount = 0. The Policy Workbench catalog then shows it as if there were no system rule.

Suggestion: apply the same rule as loadSystemPolicy() and count an empty value when its .allow_child_override key exists, with a test for it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow system policies to enforce an explicitly configured default value

2 participants